[image_picker] Migrate iOS to Pigeon and improve state handling #5285

stuartmorgan · 2022-04-15T20:29:51Z

Migrates the iOS implementation to a package-internal method channel based on Pigeon.

The driver for adopting pigeon here is that there are shared codepaths used for all three of the native method calls (across async callbacks, with state tracked in ivars), some of which return a single path (String) and some of which return multiple paths (List<String>). Currently making sure the right result type is passed entirely manual—which, as the fixed issue below demonstrates, is error prone. Pigeon allows us to have enforced type safety on the return type, so that it's impossible to accidentally call it the wrong way.

Incidental improvements made while updating the ObjC implementation:

Consolidated the state tracking to a single ivar which is a data class containing all relevant state.
Slightly reduced the reliance on the state ivar: it is now only set just before doing an async call; up until then the state is passed explicitly between methods. This makes the flow of the initial steps clearer, and helps avoid future cases like those we have had before where subtle ordering bugs in the initial synchronous method calls could cause incorrect behavior.
Consolidated all result calls to helpers: I noticed while adjusting some of the result calls to match the new callback signature that we had paths where we would return an error but not clear the state object. That would cause the next call to return a second error on the same result, which is a violation of the platform channel API. Now all errors, as well as all cancel paths, use a helper like the previously existing success case helper, which also clears state.

Not in scope:

Eliminating the use of an ivar to track state in first place (vs. using associated objects or similar to access it by controller from the async callbacks).

Part of flutter/flutter#94224
Fixes flutter/flutter#100132

Pre-launch Checklist

stuartmorgan · 2022-04-15T20:39:00Z

Some review notes:

@jmagman Feel free to delegate if there's someone who wants to start taking ownership of this iOS plugin implementation.
@gaaclarke I added you mostly for review of the Pigeon bits, but if you'd like to review more, the more eyes the better
CI will be totally broken for the moment because it references Pigeon 3.0.2, which doesn't exist yet. I'll re-run tests once that's reviewed and published. The published version of Pigeon makes invalid Dart from this file.
For anyone new to Pigeon: none of the files that have a comment at the top about being auto-generated by Pigeon need to be reviewed (unless you want to give feedback about Pigeon itself).
I deliberately changed the tests as little as possible so that the new implementation would be validated against the existing tests. Notes:
- The one removed iOS test is because passing nil to the helper being tested there is now allowed (since cancel flows use it).
- The other iOS changes are just to change the way the call is structured so that it's using the Pigeon API instead of raw serialization (which is a Pigeon implementation detail, so would be fragile to Pigeon updates).
- While the Dart tests look new, they are actually a duplicate of the existing tests of the shared method channel implementation, changed as little as I could manage to make them work with Pigeon. The copy and then update are separate individual commits in the PR, so reviewing the test file changes commit that adopts Pigeon on the Dart side, rather than reviewing the entire test file, is probably more useful.

gaaclarke

The pigeon code looks good. I gave everything a look for clerical errors and didn't notice anything. This is definitely a big improvement. My one big nit is that the plugin is still storing a closure which is error prone and I mention in my comments above. It would be easier to work with and verify as correct if we were instead tying together callbacks and never storing the callback in an instance variable.

gaaclarke · 2022-04-18T16:43:12Z

packages/image_picker/image_picker_ios/ios/Classes/FLTImagePickerPlugin.m

  UIImagePickerController *imagePickerController = [self createImagePickerController];
  imagePickerController.modalPresentationStyle = UIModalPresentationCurrentContext;
  imagePickerController.delegate = self;
  imagePickerController.mediaTypes = @[ (NSString *)kUTTypeImage ];
+  self.callContext = context;


Setting this as an instance variable makes it hard to verify that the result is called in all cases. If all the branches to this method had callbacks for their asynchronous operations, we could see that they all correctly call the result back or not.

Yes, that part is still a mess. This is a direct replacement for the existing ivar state (reply, arguments), which had the same problem, just now consolidated to one object to make bookkeeping easier.

The reason I decided to explicitly make changing that not in scope (see PR description) is that I wanted to avoid turning the PR into a full rewrite. The pigeon changes are already substantial, but this keeps the actual logic flows mostly the same to limit the scope of what could be broken, and make the review more manageable.

We should absolutely fix that later.

gaaclarke · 2022-04-18T16:51:14Z

packages/image_picker/image_picker_ios/ios/Classes/FLTImagePickerPlugin.m

- (void)handleSavedPathList:(NSArray *)pathList {
-  if (!self.result) {
+- (void)returnSavedPathList:(nullable NSArray *)pathList {
+  if (!self.callContext) {


Should we silently fail in these cases? This seems like a logical error if we get here, no?

Probably; this falls under the umbrella of leaving most of the code flows the same. I didn't want to risk introducing a new client-visible error here if there's a latent logic bug in the unchanged overall flow that this has been masking.

Fixing the use of ivars will eliminate the need for this method in the first place, so it'll get fixed then.

gaaclarke · 2022-04-18T16:57:31Z

packages/image_picker/image_picker_ios/lib/image_picker_ios.dart

+    return path != null ? PickedFile(path) : null;
+  }
+
+  Future<String?> _getVideoPath({


nit: naming is a bit confusing since this says "getVideoPath" but is calling "pickVideo"

There's some weird legacy naming in this plugin from the way things were named when the platform interface was created, and then when the new cross-file version were implemented. (This is just a copy from the shared method channel.)

I'll give it a better name.

Standardized the helpers as _<host API name>AsPath. Still not great, but consistent, and it makes it more clear what the distinction is between the helper (returns paths) and the public variants (return XFile or PickedFile).

gaaclarke · 2022-04-18T17:19:23Z

packages/image_picker/image_picker_ios/test/image_picker_ios_test.dart

+  bool operator ==(Object other) {
+    return other is _LoggedMethodCall &&
+        name == other.name &&
+        mapEquals(arguments, other.arguments);
+  }


tangent: Dart equality is kind of a mess. You never know if == means identity or value equality. I know there is prior art for mixing these up.

gaaclarke · 2022-04-18T17:21:18Z

packages/image_picker/image_picker_ios/test/image_picker_ios_test.dart

+  }
+}
+
+class _ApiLogger implements TestHostImagePickerApi {


Can't you use a mocking library for this?

Under this testing structure, a mocking library doesn't help; I'm already implementing all of the methods to log them anyway.

If I completely restructured the tests, then yes, but as mentioned in my initial PR comments I deliberately changed the tests as little as I could in this PR in order to ensure that implementations didn't regress something in a way that was masked by test change mistakes.

stuartmorgan · 2022-04-20T13:56:14Z

Pigeon 3.0.2 has been published, so CI is now green.

stuartmorgan · 2022-04-20T17:16:24Z

Switching iOS owners review to @cyanglaz per new code owner assignments.

packages/image_picker/image_picker_ios/ios/Classes/FLTImagePickerPlugin.m

cyanglaz · 2022-04-22T17:47:47Z

packages/image_picker/image_picker_ios/ios/Classes/FLTImagePickerPlugin.m

 */
- (void)handleSavedPathList:(NSArray *)pathList {
-  if (!self.result) {
+- (void)returnSavedPathList:(nullable NSArray *)pathList {


nits: The method name suggests it should return the list. Is there a better way to rename it so that it is clearer that this method is sending the result to method channel. (I can't think of any though)
If no better name is found, maybe update the doc so it is more explicit:

"then returns it in callContext.result of the original method channel call"

Renamed both of the response methods to sendCallResultWith*, and also updated the comments per your suggestion, so they should be much more explicit about what they are doing.

(Long term, the real solution is to get rid of these methods.)

cyanglaz

LGTM % nits

…ling (flutter/plugins#5285)

…ter#5285)

stuartmorgan added 7 commits April 12, 2022 16:27

Switch to internal copy of method channel implementation

8961c9b

Remove Android-only functions

761d926

Temporarily point to local pigeon

99f75e0

Add Pigeon API, and update Dart to use it

9950255

Update ObjC implementation to use Pigeon

ebb4d84

Autoformat generated Pigeon code

8fd936a

Reference Pigeon 3.0.2, not yet published

c4e2963

stuartmorgan requested review from jmagman, BeMacized and gaaclarke April 15, 2022 20:29

github-actions bot added p: image_picker platform-ios labels Apr 15, 2022

Version bump

f429c60

gaaclarke approved these changes Apr 18, 2022

View reviewed changes

Standardize Dart helper names

49e175c

stuartmorgan requested review from cyanglaz and removed request for jmagman April 20, 2022 17:16

cyanglaz reviewed Apr 22, 2022

View reviewed changes

cyanglaz approved these changes Apr 22, 2022

View reviewed changes

Review comments

fe9980c

stuartmorgan added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Apr 22, 2022

stuartmorgan mentioned this pull request Apr 22, 2022

[image_picker] Fix pickMultiImage throwing an error on iOS 13 and below. #5264

Closed

11 tasks

fluttergithubbot merged commit 10c8f9e into flutter:main Apr 22, 2022

engine-flutter-autoroll mentioned this pull request Apr 25, 2022

Roll Plugins from ca63d964d9fd to 4ff0157340ea (3 revisions) flutter/flutter#102511

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 25, 2022

10c8f9e3f [image_picker] Migrate iOS to Pigeon and improve state hand…

4c7c62f

…ling (flutter/plugins#5285)

stuartmorgan mentioned this pull request Jun 13, 2022

[camera] Switch to internal method channels #5943

Merged

11 tasks

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[image_picker] Migrate iOS to Pigeon and improve state handling (flut…

7f13e35

…ter#5285)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[image_picker] Migrate iOS to Pigeon and improve state handling #5285

[image_picker] Migrate iOS to Pigeon and improve state handling #5285

stuartmorgan commented Apr 15, 2022

stuartmorgan commented Apr 15, 2022

gaaclarke left a comment

gaaclarke Apr 18, 2022

stuartmorgan Apr 18, 2022 •

edited

Loading

gaaclarke Apr 18, 2022

stuartmorgan Apr 18, 2022

gaaclarke Apr 18, 2022

stuartmorgan Apr 18, 2022

stuartmorgan Apr 18, 2022

gaaclarke Apr 18, 2022

gaaclarke Apr 18, 2022

stuartmorgan Apr 18, 2022

stuartmorgan commented Apr 20, 2022

stuartmorgan commented Apr 20, 2022

cyanglaz Apr 22, 2022

stuartmorgan Apr 22, 2022

cyanglaz left a comment

[image_picker] Migrate iOS to Pigeon and improve state handling #5285

[image_picker] Migrate iOS to Pigeon and improve state handling #5285

Conversation

stuartmorgan commented Apr 15, 2022

Pre-launch Checklist

stuartmorgan commented Apr 15, 2022

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Apr 20, 2022

stuartmorgan commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

stuartmorgan Apr 18, 2022 •

edited

Loading